-
-
Notifications
You must be signed in to change notification settings - Fork 505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
4653: Permit :date_range filter param in DistributionsController#index #4776
4653: Permit :date_range filter param in DistributionsController#index #4776
Conversation
Looks good from a functional pov -- Over to @dorner for technical comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be handled by a request test? Controller tests are kind of missing some of the extra validations and coverage that request tests give us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it to a request spec. I had figured that this permit shoulda-matcher on the controller was enough, but I guess with a request spec I could also ensure no "unpermitted params" messages are being logged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the controller spec to a request spec. Let me know if this is the kind of implementation you had in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't think I was looking closely enough. My apologies. I don't think we need a spec just to test that a particular log is or isn't written. If this isn't affecting actual application behavior, I'd rather not have a spec at all.
…d-param-error-in-distributions-controller
Removed the spec. Yea, I was a little confused when writing it :) |
Looks good, thanks! |
rubyforgood#4776) * 4653: Permit :date_range filter param in DistributionsController#index * Move controller spec to request spec * Remove unneeded specs
@coalest: Your PR |
Resolves #4653
Description
This "unpermitted parameter" warning was logged because we were permitting other filter params but not the
date_range
param. This parameter is needed for date filtering, so I added it to the permitted params list.Type of change
Minor change in logging.
How Has This Been Tested?
Added a spec to test permitted filter params.
And manually noticed that the "unpermitted_parameter" warning was no longer being logged.